Swift: Model withUnsafeBytes and similar closure methods by geoffw0 · Pull Request #13827 · github/codeql

geoffw0 · 2023-07-27T13:49:45Z

(draft PR because this is currently sitting atop an early version of #13741 that is necessary to get some of the results; please ignore those commits, they will be rebased away once that PR is merged)

Model withUnsafeBytes and some similar closure methods.

Limitations:

I've used .ArrayElement to access content when we know the Collection is an Array. I've used two models - one with .ArrayElement and one with no content specifier - when the object could be an Array or another type of Collection. I've used no content specifier for pointers (UnsafePointer etc) since we don't have content for any of those (yet). Thus, some level of conflation (e.g. between ptr and ptr[0]) is inevitable at this stage. We should be able to improve accuracy as we implement further types of content flow.
another limitation is that certain methods and fields used in some of the tests (e.g. .baseAddress) are not yet modelled. There is an in-progress issue for these.
I'm not sure why taint from array elements isn't always flowing into closures, e.g. in int.swift line 19. It works if the qualifier is a tainted array, but not if it's an array with tainted content as in the test (despite there being models for both cases).
- update: there's a fix in another PR that ~~very likely addresses~~ has addressed this.

TODO:

rebase atop final Swift: add DataFlow::Content for arrays #13741
add change note
~~may~~ will also benefit from 098db58 (for missing test results such as int.swift line 19)

…so define models there as well.

…t's interesting).

geoffw0 · 2023-08-08T10:00:38Z

With the content flow improvements in main now, I think this is ready to review and merge. @rdmarsh2 would you mind taking a look?

geoffw0 · 2023-08-08T16:53:55Z

(DCA LGTM)

rdmarsh2 · 2023-08-10T18:15:18Z

Should those really all be taint steps? I wonder if we could do value flow if we flowed through the UnsafeByte pointers as CollectionContent

MathiasVP · 2023-08-11T09:41:30Z

Examples such as this (from https://developer.apple.com/documentation/swift/array/withunsafebufferpointer(_:)) certainly makes it seem like we should be using some kind of Content:

let numbers = [1, 2, 3, 4, 5]
let sum = numbers.withUnsafeBufferPointer { buffer -> Int in
    var result = 0
    for i in stride(from: buffer.startIndex, to: buffer.endIndex, by: 2) {
        result += buffer[i]
    }
    return result
}

it sounds like withUnsafeBufferPointer should be using ArrayContent, though

geoffw0 · 2023-08-14T15:49:03Z

Should those really all be taint steps? I wonder if we could do value flow if we flowed through the UnsafeByte pointers as CollectionContent

Oh I see what you mean - though the container itself is not the same (it's a collection outside, but a pointer/buffer inside the closure), the pointed-to elements are identical to those in the original container (at least if the pointer / buffer is typed - so the exception would be any cases that convert to a so-called "Raw" pointer, which should just be taint flow).

Examples such as this ... certainly makes it seem like we should be using some kind of Content:

I think this PR was affected by not having CollectionElement and possibly the missing the MAD content flow bug fix at the time I wrote it. I'll update...

…ves.

geoffw0 · 2023-08-21T10:15:55Z

Updated:

merged in a more recent main
models are now using CollectionElement where appropriate
models are now using value flow where appropriate (content -> content flow in all but the with*Bytes methods)
fixed issues where the test was written incorrectly (without accounting for content)
allow subscripted reads from collection content (arrays can end up with collection content)

There's quite a lot going on in this PR now. I'll run DCA again to be safe.

geoffw0 · 2023-08-21T17:47:15Z

Just pushed a fix for the UnsafeJsEval test failure. The fix isn't perfect, because we have an ad-hoc model of .baseAddress that doesn't do content flow quite right (it's difficult to get it right). I have created a follow-up issue to come back to that.

geoffw0 · 2023-08-22T09:39:25Z

The DCA repeat-run looks fine. The PR checks all pass now. I would ideally like to merge this PR soon and before it gets any bigger...

MathiasVP

There's quite a lot of nice to have things as follow-ups, but they're all planned as future work. So in the spirit of not having this PR linger for any longer I'll just merge it now

MathiasVP · 2023-08-25T08:50:18Z

+    (
+      subscript.getBase().getType() instanceof ArrayType and
+      c.isSingleton(any(Content::ArrayContent ac))
+      or
+      c.isSingleton(any(Content::CollectionContent ac))
+    )


I hope that a future PR can either collapse ArrayContent and CollectionContent into a single IPA branch, or that we can ensure that a given read/store step always reads a single Content. But obviously, that's not for this PR to handle. So this LGTM for now

I agree 100%.

MathiasVP · 2023-08-25T08:52:49Z

+   *  - elements of collections, such as `Set<Element>`.
+   *  - elements of buffers, such as `UnsafeBufferPointer<Element>`.
+   *  - the pointee of a pointer, such as `UnsafePointer<Pointee>`.


It may be a small performance win to split up these three things into separate Contents in the future. But we can delay that for now, obviously.

UnsafeBufferPointer is in fact yet another type of Collection, so it falls into the existing discussion about merging or splitting collection content types properly.

UnsafePointer appears to not be a collection though. I'll create an issue for giving it its own content type.

MathiasVP · 2023-08-25T08:54:36Z

+    (
+      isSink(node)
+      or
+      isAdditionalFlowStep(node, _)


Hmmm.... Why do we need implicit read steps at the additional flow steps, and not just at the sink? I assume this is related to the ad-hoc model you had to provide for .baseAddress?

Yes I think that was why, and there's an issue for turning that ad-hoc model into a general one and getting rid of the remaining special cases here.

MathiasVP · 2023-08-25T08:55:09Z

+      c.getAReadContent() instanceof DataFlow::Content::ArrayContent or
+      c.getAReadContent() instanceof DataFlow::Content::CollectionContent


Once again, I really hope we can merge these eventually. We're straining the dataflow library quite a lot with these duplications (i.e., if we have a path that should have an access path like [a, CollectionContent, b, CollectionContent] we're really forcing the dataflow library to have:

[Field[a], CollectionContent, Field[b], CollectionContent] [Field[a], CollectionContent, Field[b], ArrayContent] [Field[a], ArrayContent, Field[b], CollectionContent] [Field[a], ArrayContent, Field[b], ArrayContent]

geoffw0 · 2023-08-29T07:55:06Z

Thanks for reviewing and merging!

geoffw0 added the Swift label Jul 27, 2023

github-actions Bot added the documentation label Jul 27, 2023

geoffw0 added 9 commits August 4, 2023 09:18

Swift: Add tests of with* closure methods.

63c71f0

Swift: Add tests with some different container types.

feadd71

Swift: Remove special case from UnsafeJsEval query.

315cb32

Swift: Model update(repeating:), to support the tests.

49d1556

Swift: Add closure function models.

664dc01

Swift: Fix mistake in the string taint test.

c48d474

Swift: Array.withUnsafeBytes doesn't reliably match ContiguousBytes, …

b41d47b

…so define models there as well.

Swift: Use .ArrayElement in the models, where appropriate.

d24db3f

Swift: Change note.

af8d4e5

geoffw0 force-pushed the closuremodels branch from c302748 to af8d4e5 Compare August 4, 2023 08:45

geoffw0 marked this pull request as ready for review August 4, 2023 08:47

geoffw0 requested a review from a team as a code owner August 4, 2023 08:47

geoffw0 added 3 commits August 8, 2023 09:40

Merge branch 'main' into closuremodels

063ab1c

Swift: Accept test changes resulting from merge.

6ccf47e

Swift: Correct a test case (but preserve the original as well since i…

c954324

…t's interesting).

geoffw0 added 8 commits August 17, 2023 17:16

Merge branch 'main' into closuremodels

59e2b04

Swift: Update models with CollectionElement, value flow.

1ac9d2c

Swift: Allow subscript content reads from collections.

0fd4f61

Swift: Accept fixed spurious test results.

86a73fa

Swift: Address pointer/pointee conflation in the string tests themsel…

a6f29fa

…ves.

Swift: Add content to the string models.

b4db68a

Swift: Fix mysterious taint flow issue.

a54747f

Swift: Minor test .expected changes.

997984c

geoffw0 added 3 commits August 21, 2023 11:28

Swift: UnsafeJSEval regression.

6ef6be7

Swift: Create proper models for JavaScriptCore.

317757b

Swift: 'good enough' fix for UnsafeJsEval flow.

f7776f8

MathiasVP approved these changes Aug 25, 2023

View reviewed changes

MathiasVP merged commit 2fd627b into github:main Aug 25, 2023

		c.getAReadContent() instanceof DataFlow::Content::ArrayContent or
		c.getAReadContent() instanceof DataFlow::Content::CollectionContent

Conversation

geoffw0 commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffw0 commented Aug 8, 2023

Uh oh!

geoffw0 commented Aug 8, 2023

Uh oh!

rdmarsh2 commented Aug 10, 2023

Uh oh!

MathiasVP commented Aug 11, 2023

Uh oh!

geoffw0 commented Aug 14, 2023

Uh oh!

geoffw0 commented Aug 21, 2023

Uh oh!

geoffw0 commented Aug 21, 2023

Uh oh!

geoffw0 commented Aug 22, 2023

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geoffw0 commented Jul 27, 2023 •

edited

Loading